-
Notifications
You must be signed in to change notification settings - Fork 6.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce ft5336 touch panel driver and integrate into lvgl #22055
Conversation
lib/gui/lvgl/lvgl.c
Outdated
}; | ||
|
||
static K_MEM_SLAB_DEFINE(kscan_slab, sizeof(struct kscan_fifo_data), 10, 4); | ||
static K_FIFO_DEFINE(kscan_fifo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not using a k_msgq here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no reason, thanks for the suggestion. i've updated the pr to use a msgq.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. The API change is fairly safe.
4224336
to
a0a82b8
Compare
Extends the keyboard scan callback row and column arguments from 8-bits to 32-bits to support a touch panel driver implementation. Signed-off-by: Maureen Helm <maureen.helm@nxp.com>
Adds device tree bindings for the focaltech ft5336 touch panel controller, which will be used on several i.mx rt evk boards. Moves address-cells and size-cells properties from the base kscan bindings to the specific microchip,xec bindings since they are not required for the ft5336. Signed-off-by: Maureen Helm <maureen.helm@nxp.com>
Introduces a new ft5336 touch panel driver for the keyboard scan (kscan) interface. The driver currently uses a timer to periodically poll touch data in the system work queue, but later it can be enhanced to use a gpio interrupt instead of a timer. Signed-off-by: Maureen Helm <maureen.helm@nxp.com>
a0a82b8
to
209bf8d
Compare
rebased to pick up #22056 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good stop-gap solution until we have a real touch screen API, but we should make clear in the Kconfig that kscan is used for a pointer like device and not a keyboard.
lib/gui/lvgl/Kconfig
Outdated
help | ||
Enable keyboard scan input | ||
|
||
config LVGL_KSCAN_DEV_NAME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if LVGL_KSCAN_DEV_NAME
should be renamed to LVGL_POINTER_KSCAN_DEV_NAME
to make it clear that it is a pointer like input device and not a real keyboard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
lib/gui/lvgl/Kconfig
Outdated
|
||
config LVGL_KSCAN_DEV_NAME | ||
string "Keyboard scan input device name" | ||
depends on LVGL_KSCAN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put LVGL_KSCAN_DEV_NAME
and LVGL_KSCAN_MSGQ_COUNT
between if LVGL_KSCAN
instead of using depends on LVGL_KSCAN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
lib/gui/lvgl/lvgl.c
Outdated
#ifdef CONFIG_LVGL_KSCAN | ||
K_MSGQ_DEFINE(kscan_msgq, sizeof(lv_indev_data_t), | ||
CONFIG_LVGL_KSCAN_MSGQ_COUNT, 4); | ||
static lv_indev_drv_t indev_drv; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to make this global variable, it can just be a local variable in lvgl_kscan_init
as lv_indev_drv_register
makes a copy of the variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Adds support for an optional lvgl touch input device using the zephyr keyboard scan interface. This can be used with the ft5336 touch panel driver, which returns single touch coordinates through the kscan driver callback. Signed-off-by: Maureen Helm <maureen.helm@nxp.com>
209bf8d
to
751a072
Compare
# Copyright (c) 2020 NXP | ||
# SPDX-License-Identifier: Apache-2.0 | ||
|
||
CONFIG_I2C=y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This board config files are a maintenance overhead sometimes. Why not enable it in soc/arm/nxp_imx/rt/Kconfig.defconfig.series ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't implement it originally because I was worried about complex Kconfig dependencies, but given your feedback I went ahead and did it now. Please have a look at the updated version.
samples/display/lvgl/src/main.c
Outdated
@@ -30,7 +31,10 @@ void main(void) | |||
return; | |||
} | |||
|
|||
hello_world_label = lv_label_create(lv_scr_act(), NULL); | |||
hello_world_button = lv_btn_create(lv_scr_act(), NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me test it on different displays, probably button could be conditional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The button does not look good on monochrome displays because of the background gradient, I suggest to use the button only if KSCAN is enabled, e.g.:
diff --git a/samples/display/lvgl/src/main.c b/samples/display/lvgl/src/main.c
index c40808258d..c566b52922 100644
--- a/samples/display/lvgl/src/main.c
+++ b/samples/display/lvgl/src/main.c
@@ -31,10 +31,14 @@ void main(void)
return;
}
- hello_world_button = lv_btn_create(lv_scr_act(), NULL);
- lv_obj_align(hello_world_button, NULL, LV_ALIGN_CENTER, 0, 0);
+ if (IS_ENABLED(CONFIG_KSCAN)) {
+ hello_world_button = lv_btn_create(lv_scr_act(), NULL);
+ lv_obj_align(hello_world_button, NULL, LV_ALIGN_CENTER, 0, 0);
+ hello_world_label = lv_label_create(hello_world_button, NULL);
+ } else {
+ hello_world_label = lv_label_create(lv_scr_act(), NULL);
+ }
- hello_world_label = lv_label_create(hello_world_button, NULL);
lv_label_set_text(hello_world_label, "Hello world!");
lv_obj_align(hello_world_label, NULL, LV_ALIGN_CENTER, 0, 0);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit awkward to conditionalize the sample like this. What if we instead configured monochrome displays to use the mono theme by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit awkward to conditionalize the sample like this.
no preprocessor macros are awkward, if (IS_ENABLED(CONFIG_OPTION)) {
is fine.
What if we instead configured monochrome displays to use the mono theme by default?
yes, an option. But it looks anyway ugly on small displays, the button size does not scale, probably lv_btn_set_fit
would help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lv_btn_set_fit(hello_world_button, LV_FIT_TIGHT);
did not work well, the button is still too big for a 128x64 pixel display.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not fond of making the button conditional in a simple "hello world" type of example, but I don't have a 128x64 pixel display to play with making the button fit. I updated the patch to your suggestion.
Adds device tree nodes and configures Kconfig defaults for the ft5336 touch panel driver on mimxrt10{50,60,64}_evk boards. Signed-off-by: Maureen Helm <maureen.helm@nxp.com>
751a072
to
03dca4c
Compare
Enhances the lvgl sample to support an optional touch panel input on mimxrt10{50,60,64}_evk boards. Signed-off-by: Maureen Helm <maureen.helm@nxp.com>
03dca4c
to
1f2935f
Compare
This is another attempt to enable the touch panel driver on i.mx rt boards and integrate into LittlevGL. Unlike #16119 which uses the experimental zio interface in the topic-sensors branch, this version uses the existing kscan interface already in master.
One small change was required in the kscan api, extending row and column callback arguments from 8-bits to 32-bits.